Introduced Nope::VersionNotFound error #1043
Conversation
jyn514
added
A-backend
src/web/mod.rs
Outdated
| corrected_name, | ||
| version: MatchSemver::Exact((version.to_owned(), *id)), | ||
| }); | ||
| } | ||
|
|
||
| // Now try to match with semver | ||
| let req_sem_ver = VersionReq::parse(&req_version).ok()?; | ||
| let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::CrateNotFound)?; |
There was a problem hiding this comment.
Shouldn't this be VersionNotFound?
| let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::CrateNotFound)?; | |
| let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::VersionNotFound)?; |
There was a problem hiding this comment.
Please also add a test for this case by requesting /:crate/some_version/, then make sure that it fails before and succeeds after this change.
|
Can you also add some tests for the new behavior? There's examples around https://github.com/rust-lang/docs.rs/blob/master/src/web/rustdoc.rs#L821 - I don't think anything tests the error text currently, but it shouldn't be too hard to get it from the response: https://docs.rs/reqwest/0.10.8/reqwest/struct.Response.html#method.text |
jyn514
added
S-waiting-on-author
Tests are still TODO
Tests are still TODO
Trying to reproduce the issue rust-lang#55 revealed another bug: the order in which the different handlers were called in CratesfyiHandler::handle resulted in the more specific errors from the router_handler being dropped, due to them being 404 and the last handler being the static_handler which would just return a ResourceNotFound error. It should be noted that the different execution order might affect the performance in production.
Tests are still TODO
|
Looks like you need to update the text for one of the tests: |
|
Yea, I've already updated it. I was just confused for a moment because the failing test wasn't present in my branch so I had to fetch the origin and rebase. |
|
Sounds great! Thanks so much for working on this :) |
jyn514
added
S-waiting-on-review
| // routes_handler which is currently run last. This means that `get("resource.exe")` will | ||
| // fail with a `no so such crate` instead of 'no such resource' |
There was a problem hiding this comment.
Ooh, good catch. This seems like an ok bug to have though - if you're trying to download .exes docs.rs doesn't want to help you even as much to have the right error.
| .next() | ||
| .unwrap() | ||
| .text_contents(), | ||
| "The requested version does not exist", |
There was a problem hiding this comment.
Eventually it would be nice to instead say 'all versions of this crate are yanked', which makes more sense since this didn't request a specific version. But that can be a follow-up PR.
|
Thanks for the help and quick, detailed reviews 😊 |
Trying to reproduce the issue #55 revealed another bug: the order in which the different handlers were called in CratesfyiHandler::handle resulted in the more specific errors from the router_handler being dropped, due to them being 404 and the last handler being the static_handler which would just return a ResourceNotFound error.
It should be noted that the different execution order might affect the performance in production.